Skip to content

fix(uploadstore): bound R2 Serve body read - #182

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f001-r2-body-deadline
Open

fix(uploadstore): bound R2 Serve body read#182
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f001-r2-body-deadline

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where users opening an R2-backed upload (image preview, file download, range seek) would see the request hang forever when Cloudflare R2 accepted the GET, returned headers, and then stopped sending body bytes. The outbound client only bounds the wait for response headers. After headers arrive, ServeHTTP copies the body with Client.Timeout left at 0, so a stalled origin never unblocks the download handler.

This is the R2 serve path used by GET /api/uploads/:upload_id, not the browser fetch timeout from #168 and not a change to the streaming-safe overall client timeout.

Why This Change Was Made

ServeHTTP now wraps the R2 response body with an idle read deadline (30s). Each Read that makes no progress in that window closes the body and returns r2 serve: body read stalled. Bytes that keep arriving reset the idle timer, so large or slow-but-live objects still stream.

http.Client.Timeout stays 0, matching TestR2ConfigValidation and the #168 contract. The empty DefaultTransport fallback is left unchanged.

User Impact

A hung R2 object body fails the download after 30s of idle instead of pinning the upload handler until the user disconnects. Successful and progressing streams are unchanged.

Evidence

Before the patch, ServeHTTP stayed in io.Copy after the origin flushed headers and sent no body. A 15s process timeout still had the copy blocked:

panic: timed out after 15s
io.Copy(...)
github.com/openclaw/clickclack/apps/api/internal/uploadstore.(*R2).ServeHTTP
FAIL

After the patch, the same stalling origin against production NewR2 (default 30s idle, Client.Timeout still 0) returns instead of hanging:

$ go run ./apps/api/cmd/r2stallproof
origin_headers=flushed
elapsed_ms=30054
err=r2 serve: body read stalled

Client.Timeout is still 0 (Timeout != 0 would fail TestR2ConfigValidation). A 50ms idle on the same Serve path returns r2 serve: body read stalled in 0.10s.

Real behavior proof

  • Behavior or issue addressed: R2 ServeHTTP hung forever when the origin sent response headers and then stalled the object body, because Client.Timeout is 0 and only ResponseHeaderTimeout was set.

  • Real environment tested: macOS Darwin 25.6.0 arm64, go1.27.0, branch fix/f001-r2-body-deadline at the patched tree /tmp/oc-pr-clickclack-F001.

  • Exact steps or command run after this patch:

    cd /tmp/oc-pr-clickclack-F001
go run ./apps/api/cmd/r2stallproof

    The program built NewR2 against a local origin that flushed a 200 with Content-Length: 100 and then sent no bytes.

  • Evidence after fix: terminal output from the patched tree:

    $ go run ./apps/api/cmd/r2stallproof
origin_headers=flushed
elapsed_ms=30054
err=r2 serve: body read stalled

  • Observed result after fix: After headers flushed, ServeHTTP returned r2 serve: body read stalled at 30054ms (the 30s idle bound). It did not remain blocked on the body copy. Client.Timeout remains 0.

  • What was not tested: Live Cloudflare R2 credentials and a production bucket GET. Save/Delete paths are unchanged.

Related

@SebTardif
fix(uploadstore): bound R2 Serve body read
3459131 
R2 ServeHTTP copies the object body with Client.Timeout 0. Response
headers are bounded, but a stalled body after headers left the
download handler blocked.

Wrap the Serve response body with an idle read deadline so a hung
origin unblocks the request without changing the streaming-safe
client timeout.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested a review from a team as a code owner August 29, 2026 18:56
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 29, 2026, 2:59 PM ET / 18:59 UTC.

ClawSweeper review

What this changes

The PR adds a 30-second idle deadline around R2 object-body reads so R2-backed upload downloads do not wait indefinitely after response headers arrive.

Merge readiness

⚠️ Needs maintainer review before merge - 3 items remain

Keep open: the idle timeout bounds the stalled R2 read, but it now returns an error after the upload response has already started, causing the API handler to append a JSON error to a partial file response.

Priority: P2
Reviewed head: 3459131e309bcdf1a64429dd4de7dc48494db7d0

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The stalled-read recovery is demonstrated, but the post-header error path can still produce an invalid download response.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
Evidence reviewed 5 items Current main leaves body reads unbounded: The fetched main revision calls io.Copy directly on the R2 response body after writing the upload status, without a body-read deadline.
Introduced timeout returns a post-header error: The PR wraps the response body before ServeHTTP writes the successful response; the wrapper closes a stalled body and returns errServeBodyStalled from io.Copy.
API error writer runs after serving errors: getUpload handles every upload-store error with writeError, while writeError calls writeJSON; for a stalled stream this happens after R2 ServeHTTP has already called WriteHeader and may have copied bytes.
Findings 1 actionable finding [P2] Avoid appending JSON errors to a started upload response
Security None None.

How this fits together

ClickClack’s upload store retrieves R2-backed files for the authenticated upload endpoint. The endpoint starts the client response before streaming the R2 body, and later read errors return through the API handler.

flowchart LR
  A[Authenticated upload request] --> B[Upload API handler]
  B --> C[R2 upload store]
  C --> D[R2 response body]
  D --> E[Stream bytes to client]
  D --> F[Idle-read timeout]
  F --> G[Post-header error handling]
Loading

Before merge

  • Avoid appending JSON errors to a started upload response (P2) - The new wrapper turns a stalled body into an error after ServeHTTP has already written the R2 status and may have copied bytes. getUpload then calls writeError, which serializes JSON to that same response; clients can receive partial file data followed by API error bytes under the original successful status. Track response commitment or return a stream-termination outcome that the endpoint does not serialize.
  • Resolve merge risk (P1) - An existing R2 deployment that stalls after a partial response can receive a successful file status with appended JSON error bytes, producing a corrupt or ambiguous download rather than a clean failed stream.
  • Complete next step (P2) - Repair the introduced post-header error handling before merge; the narrow endpoint and regression test are clear.

Findings

  • [P2] Avoid appending JSON errors to a started upload response — apps/api/internal/uploadstore/r2.go:192
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 2 files; production +60/-17, tests +62/-0 The patch adds one response-body wrapper and one direct store-level stalled-read test.

Merge-risk options

Maintainer options:

  1. Handle post-header stream errors (recommended)
    Prevent the upload handler from serializing an HTTP error after the object response has already started, then cover a partially streamed stalled body.
  2. Accept truncated-download behavior
    Land the idle bound while accepting that clients may receive a partial response when R2 stalls after headers.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Preserve the established upload response once headers are committed; add an end-to-end stalled-body regression test that proves no JSON error bytes are appended.

Technical review

Best possible solution:

Preserve a clean terminated download after a post-header R2 read failure and add an endpoint-level regression test for a partially delivered stalled object.

Do we have a high-confidence way to reproduce the issue?

Yes: current-main source shows the unbounded post-header io.Copy path, and the submitted trace exercises the corresponding stalled-body scenario against NewR2.

Is this the best way to solve the issue?

No: bounding the read is appropriate, but the endpoint must not write an HTTP error after file headers and bytes have begun.

Full review comments:

  • [P2] Avoid appending JSON errors to a started upload response — apps/api/internal/uploadstore/r2.go:192
    The new wrapper turns a stalled body into an error after ServeHTTP has already written the R2 status and may have copied bytes. getUpload then calls writeError, which serializes JSON to that same response; clients can receive partial file data followed by API error bytes under the original successful status. Track response commitment or return a stream-termination outcome that the endpoint does not serialize.
    Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 486fd23545af.

Labels

Label changes:

  • add P2: This is a bounded reliability change to R2-backed upload downloads.
  • add merge-risk: 🚨 compatibility: The timeout can return an error after a successful file response has been committed to existing clients.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.

Label justifications:

  • P2: This is a bounded reliability change to R2-backed upload downloads.
  • merge-risk: 🚨 compatibility: The timeout can return an error after a successful file response has been committed to existing clients.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.

Evidence

Acceptance criteria:

  • [P1] go test ./apps/api/internal/uploadstore.
  • [P1] go test ./apps/api/internal/httpapi.

What I checked:

  • Current main leaves body reads unbounded: The fetched main revision calls io.Copy directly on the R2 response body after writing the upload status, without a body-read deadline. (apps/api/internal/uploadstore/r2.go:209, 486fd23545af)
  • Introduced timeout returns a post-header error: The PR wraps the response body before ServeHTTP writes the successful response; the wrapper closes a stalled body and returns errServeBodyStalled from io.Copy. (apps/api/internal/uploadstore/r2.go:192, 3459131e309b)
  • API error writer runs after serving errors: getUpload handles every upload-store error with writeError, while writeError calls writeJSON; for a stalled stream this happens after R2 ServeHTTP has already called WriteHeader and may have copied bytes. (apps/api/internal/httpapi/features.go:525, 3459131e309b)
  • R2 behavior history: The current ServeHTTP lines trace to the R2 feature history, with the original storage implementation and outbound-header timeout work authored by Peter Steinberger and the current body carried through a later refactor by Shakker. (apps/api/internal/uploadstore/r2.go:175, 711f893befdd)
  • Submitted real behavior proof: The PR body includes a patched-tree macOS terminal trace using production NewR2 against a local origin that flushes headers then stalls; it reports err=r2 serve: body read stalled after about 30 seconds. (3459131e309b)

Likely related people:

  • Peter Steinberger: Introduced Cloudflare R2 storage and the subsequent outbound upload timeout behavior in the R2 implementation history. (role: introduced R2 storage and header-timeout behavior; confidence: high; commits: faacf69ae08b, f7a10e989706; files: apps/api/internal/uploadstore/r2.go)
  • Shakker: Current-main blame attributes the surviving R2 ServeHTTP body to this refactor-era commit. (role: current serve-path history contributor; confidence: medium; commits: 711f893befdd; files: apps/api/internal/uploadstore/r2.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add an authenticated endpoint-level test showing a stalled R2 response cannot append JSON error bytes after upload streaming begins.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@SebTardif SebTardif mentioned this pull request Aug 30, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@SebTardif